The Big Merge #30

ScottSoren · 2021-12-07T18:38:03Z

Here it is, the big PR.

This contains user-ready tools for analyzing data from

Electorchemistry
Mass spectrometry (MID) including calibrations
Electrochemistry - Mass Spectrometry
Spectroelectrochemistry (UV-vis)

It also contains the documentation viewable at https://ixdat.readthedocs.org

It has been tested against:

flake8
pytest. I.e. all the tests in tests/
all of the scripts in development_scripts/. Importantly this means that all readers work.
The scripts in Huang2021: https://github.com/ScottSoren/Huang2021/tree/ixdat_v0p2, which plot and analyze calibrated EC-MS data

It has not yet been tested against other tutorials and article repositories, but I imagine that will not result in huge changes for ixdat.

I suggest viewing source modules in the following order:

db.py
backends/
measurements.py
techniques/ec.py
techniques/cv.py
techniques/ms.py
techniques/ec_ms.py
spectra.py
plotters/
readers/

Challenges to focus on include:

the relationship of measurements to their calibrations
the relationship of measurements to their backends
the implementation of MS calibration and how it can be adapted to make use of evt. powerful external packages
the readability of the code

Note that while spectroelectrochemistry.py is not a priority here (Caiwu gave the module as well as the corresponding plotter and readers a thorough review in #13), spectra.py is important with respect to coming implementation of mass scan analysis.

I think the docs to be out of scope for a thorough review here, but the documentation does need improvement!

There are TODOs and FIXMEs spread throughout the code. Most of them have to do with a coming "metaprogramming" piece of work which will fix the class attributes of Saveable classes to serve as database design blueprint, and simplify parts of the code.

Looking forward to being back on master for distribution, docs, and everything else :D

…convolution Adding newest readers to deconvolution development

…onvolution

Soren implentnting a few fixes of his work where it's available to the deconvolution work. This merge mainly brings over TODO's added in response to WS3 and first review of #6.

Minor improvements after DWS3 and first #6 rev.

…econvolution

KennethNielsen

Hi Scott

So, I finally managed to get through it.

First of, this bears repeating from slack.

Just in case I forget to mention it in the review. With one teeny tiny exceptions, which I don't really know yet how to solve, the figure code reads like an absolute joy! :star-struck: . It reads really clean and the compos-ability brought in by the ability to pass around axis, makes the code for all the composite/(more specific) figures really nice, easy to read and without copied code and I think the few cases of factoring common procedures out (making panels and such) are just the right amount and with interface at the right abstraction level. It is incredibly good!

The one teeny-tiny thing is that plotters consequently take a default measurement and a measurement override in the plotting methods. At first I thought it was an anti-pattern (violating the "There should be one-- and preferably only one --obvious way to do it." and so I commented on it, until I realized that it seemed to actually be used and that I didn't have a good suggestion for how to do it otherwise. So when you get to those comments, just ignore them for now, except then one where I think there is a semantic problem, because we may be referencing a measurement that doesn't exist. The review is unfortunately way to big for me to find them again and delete them :(

With regards to your focus point, I didn't notice anything worth commenting on for the first three. It looks good to me. With regards to general readability I think it is good. There are a few minor exceptions, but nothing that scares me:

Maybe because we come from academia (or maybe matlab) there is a tendency in come of the code to abbreviate more than I prefer. In these days where everyone and their aunt has a auto-completing editor, I would much rather have pretty much everything written out and improve the decipherability in the process. If it is for economy of typing while writing the code, I would suggest to just search a replace when done.
There are a few places here where relatively complex formulae or (vectorized) np formulae is just there, without any explanation or reference. That may be ok, if this is just the kind of formulae that people within the field has tattooed to their body, or as long as the author is still active, but it can be a maintainability problem in the long wrong.
Finally. I think there is some more things to work with and possible iterate on with regards to composition, but that can be for another time.

All in all, this looks fantastic and I can't wait to get it merged.

README.rst

TOOLS.rst

src/ixdat/techniques/spectroelectrochemistry.py

src/ixdat/techniques/deconvolution.py

ScottSoren

I'll update periodically as I implement the review. This first one is everything outside of "src", i.e. mainly docs/ and development_scripts/
Most issues were easy-to-resolve typos (amazed how much you catch!). Unresolved comments mainly relate to the purpose of development_scripts. Let me know if you accept my explanation, and whether they are stashed in the right place for that purpose.

development_scripts/reader_testers/test_msrh_sec_reader.py

development_scripts/reader_testers/test_pfeiffer_reader.py

docs/requirements.txt

docs/source/developing.rst

docs/source/technique_docs/electrochemistry.rst

docs/source/technique_docs/mass_spec.rst

docs/source/tutorials.rst

KennethNielsen · 2022-03-10T10:30:46Z

@ScottSoren I went through all unresolved in everything but src and left just a single unresolved, which is the Jupyter/IPhython comment.

ScottSoren

Hi Kenneth, here's the next chunk of my implementation of the review, covering Exporters.

The one bit of pending implementation is removing the ability to pass a measurement into the export method. To do so, I could use your input (again) on the fundamental design discussion here:
https://github.com/ixdat/ixdat/pull/30/files#r810926968

src/ixdat/backends/directory_backend.py

src/ixdat/constants.py

src/ixdat/data_series.py

src/ixdat/db.py

src/ixdat/exporters/spectrum_exporter.py

KennethNielsen · 2022-03-11T13:09:22Z

I've completed your last implement. I suggest as for the export and plotters to leave it as is, since it works and is at worst an API annoyance, if it even is that. In any case, if we defer we can get the Big Merge in and maybe to small quick draft experiments to explore design options for it.

ScottSoren

Review of measurment.py, plotters/, readers/, and spectra.py here. Just techniques/* to go.

src/ixdat/measurements.py

src/ixdat/readers/ixdat_csv.py

src/ixdat/readers/zilien.py

src/ixdat/readers/reading_tools.py

src/ixdat/spectra.py

ScottSoren

Woohoo! Done implementing :D
Ready to merge, awaiting your approval.

src/ixdat/techniques/analysis_tools.py

src/ixdat/techniques/cv.py

src/ixdat/techniques/deconvolution.py

src/ixdat/techniques/ms.py

src/ixdat/techniques/spectroelectrochemistry.py

tasks.py

src/ixdat/techniques/cv.py

ScottSoren · 2022-03-14T14:19:30Z

Ready to merge when you change the status of the review from "changes requested".
There will be some follow-up work which I'll engage in the next days getting all tutorials and article repositories working with the updated ixdat :) - accompanied by applying depreciation warnings where relevant.
Then I'll change https://ixdat.readthedocs.org to compile from master and upload ixdat v0.2.0 to PyPI.

New contributions that were made to the user_ready branch while we were working on #30

kkrempl and others added 30 commits December 19, 2020 16:29

fix import

8fc238e

fix import

ccfb5a3

pkl readers

d45369d

Merge remote-tracking branch 'origin/reader_for_biologic_mpt' into de…

7a1ccfe

…convolution Adding newest readers to deconvolution development

EC_MS .pkl reader

4a007a0

Implementation of the deconvolution class and further functionality.

0d2f2e5

Syntax correction of comments

14a15d2

Better formatting, added TODOs, smaller bug fixes

2142121

More TODOs

2dd8a52

Even more TODOs

d6a4166

small bug fixes

e0e6e8a

bug fixes, now able to run scripts for paper

ac2e824

change docstring

e4bc226

Merge branch 'ec_features' of https://github.com/ixdat/ixdat into dec…

e5d4580

…onvolution

delete merge conflicts

17b4990

Merge branch 'ec_features' into deconvolution_sbs

0e4c453

Soren implentnting a few fixes of his work where it's available to the deconvolution work. This merge mainly brings over TODO's added in response to WS3 and first review of #6.

pass cls from M.read() via R.read() to M.from_dict()

49a68a5

update requirements and appease flake8

def6244

rename 'get_' methods for t, v vectors to 'grab_'

36e91c0

Merge pull request #7 from ixdat/deconvolution_sbs

85da96a

Minor improvements after DWS3 and first #6 rev.

small changes for NH3 measurements

27e044d

Merge branch 'deconvolution' of https://github.com/ixdat/ixdat into d…

057cca2

…econvolution

write placeholder zilien Reader and ecms Plotter

663d709

Add MSPlotter, improve EC-MS related docstrings

e5a7d86

start updating documentation

81a429f

fix documentation formatting

4a6cc29

add read_url() method to Measurement

6f70787

updates for pypi: *$ pip install ixdat*

e2060e7

add ECMeasurements.select_sweep and constants.py

8007662

integrate() and grab_of_t()

7f1a718

KennethNielsen requested changes Feb 28, 2022

View reviewed changes

ScottSoren added 2 commits March 9, 2022 14:18

aliases in EC_MS pickle importer

de4dada

implement review outside of src

299f706

ScottSoren commented Mar 9, 2022

View reviewed changes

ScottSoren added 2 commits March 9, 2022 17:28

physical constants from scipy

b2a0d86

implement review of db and data_series

0087cd0

implement review of exporters

4700a7d

ScottSoren commented Mar 11, 2022

View reviewed changes

ScottSoren added 4 commits March 13, 2022 10:32

implement review of measurement.py

0276f21

implement review of plotters

9e9e39b

implement review of readers

f806455

implement review of spectra.py

77040ba

ScottSoren commented Mar 13, 2022

View reviewed changes

implement review of techniques

76bd43b

ScottSoren commented Mar 13, 2022

View reviewed changes

ScottSoren commented Mar 14, 2022

View reviewed changes

src/ixdat/techniques/cv.py Show resolved Hide resolved

finish review: ECMS axes order and misc details

d051c8f

KennethNielsen self-requested a review March 14, 2022 14:59

KennethNielsen approved these changes Mar 14, 2022

View reviewed changes

ScottSoren merged commit 827d993 into master Mar 14, 2022

ScottSoren deleted the backend_fixes branch March 14, 2022 15:01

This was referenced Mar 15, 2022

Debug after big merge #42

Merged

New contributions #44

Merged

ScottSoren pushed a commit that referenced this pull request Mar 17, 2022

Merge pull request #44 from ixdat/new_contributions

d1fabdc

New contributions that were made to the user_ready branch while we were working on #30

This was referenced Mar 28, 2022

fix export bug #57

Merged

ixdat 0.2.0: 'ECMSExporter' object has no attribute 'default_v_list' #56

Closed

"selector" doesn't correctly combing "Ns" and "cycle_number" #14

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The Big Merge #30

The Big Merge #30

ScottSoren commented Dec 7, 2021

KennethNielsen left a comment

ScottSoren left a comment

KennethNielsen commented Mar 10, 2022

ScottSoren left a comment

KennethNielsen commented Mar 11, 2022

ScottSoren left a comment

ScottSoren left a comment

ScottSoren commented Mar 14, 2022

The Big Merge #30

The Big Merge #30

Conversation

ScottSoren commented Dec 7, 2021

KennethNielsen left a comment

Choose a reason for hiding this comment

ScottSoren left a comment

Choose a reason for hiding this comment

KennethNielsen commented Mar 10, 2022

ScottSoren left a comment

Choose a reason for hiding this comment

KennethNielsen commented Mar 11, 2022

ScottSoren left a comment

Choose a reason for hiding this comment

ScottSoren left a comment

Choose a reason for hiding this comment

ScottSoren commented Mar 14, 2022